-
Notifications
You must be signed in to change notification settings - Fork 0
feat(alfred-live): audit-first pipeline + auth hooks #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds version 0.10.2 for Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Pipeline as Command Pipeline
participant Auth as Auth Provider
participant Audit as Audit Sink
participant Handler as Command Handler
Client->>Pipeline: executeCommandLine(jsonlLine, { auth, audit })
Pipeline->>Audit: record({ phase: "attempt", ... })
Pipeline->>Auth: authorize(context)
alt auth ok
Auth-->>Pipeline: { ok: true, data: ... }
Pipeline->>Handler: validate & execute command
Handler-->>Pipeline: command result
Pipeline->>Audit: record({ phase: "result", ok: true, result })
Pipeline-->>Client: audited result
else auth denied
Auth-->>Pipeline: { ok: false, error: AuthorizationError }
Pipeline->>Audit: record({ phase: "result", ok: false, error: "AUTH_DENIED" })
Pipeline-->>Client: audited failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@alfred-live/src/command-envelope.js`:
- Around line 118-137: buildAuditEvent currently always copies preview.raw into
the event, risking sensitive or large payloads being stored; update
buildAuditEvent to avoid unconditionally including preview.raw—either only
include raw for the "attempt" phase (e.g., if (phase === 'attempt') event.raw =
preview.raw), or require an explicit opt-in flag on preview/options (e.g.,
preview.includeRaw) and otherwise omit or redact raw (e.g., event.raw =
'[REDACTED]'); apply the same rule consistently for both attempt and result
phases and update callers to pass the opt-in when raw is safe to store.
🧹 Nitpick comments (6)
alfred-live/src/audit.js (1)
65-68: A throwing sink will prevent subsequent sinks from receiving the event.If any sink in the array throws during
record(), the exception propagates and remaining sinks are skipped. Consider wrapping each call in a try/catch to ensure fan-out resilience.♻️ Suggested fix
record(event) { for (const sink of this.#sinks) { - sink?.record?.(event); + try { + sink?.record?.(event); + } catch { + // Swallow to avoid one sink breaking fan-out to remaining sinks. + } } }alfred-live/src/auth.js (2)
56-63: Self-validation of the internally constructed provider is unreachable.The
providerobject is constructed on lines 40–54 with a validauthorizefunction, sovalidateAuthProvider(provider)on line 56 will always returnnull. This block is dead code. If the intent is to validate external providers in the future, consider extracting this into a sharedwrapAuthProvider(provider)helper at that time.
37-69: Passing a non-iterable, non-string, non-nullish value totokenswill throw atnew Set(tokens).Line 38 handles
stringandnull/undefined, but a non-iterable value (e.g.,42,true) will throwTypeError: ... is not iterable. The JSDoc types this asIterable<string>, so this is a caller contract issue, but if you want full defensive handling (matching the defensive style of the rest of this file), you could add a guard.alfred-live/src/command-envelope.js (3)
158-182:authorizeCommandreturns semantically different data shapes depending on the path.When no
authprovider is supplied, line 160 returnsokResult({ allowed: true })(i.e.{ ok: true, data: { allowed: true } }). When an auth provider succeeds, it returns the provider's ownResult<unknown>. Since the caller only inspects.okand never reads.data, this is functionally harmless — but the phantom{ allowed: true }data payload is misleading and could confuse future maintainers who expect it to be meaningful.Consider returning
okResult(null)orokResult(undefined)for the no-auth shortcut, consistent withrecordAuditEvent's style at line 141.♻️ Optional: normalize the no-auth return
function authorizeCommand(auth, context) { if (!auth) { - return okResult({ allowed: true }); + return okResult(null); }
194-200: Audit recording failure silently swallows the original command result.If the
result-phase audit recording fails (line 196),encodeAuditedResultreturns an error envelope for the audit failure and discardsresultEnvelopeentirely. This means a command that succeeded will look like a failure to the caller if the audit sink throws.This appears intentional for an "audit-first" design (consistent with the attempt-phase check at lines 440-442), but it's a strong guarantee that couples operational reliability to the audit sink. Worth a brief doc comment here so future readers understand the trade-off.
184-192:buildAuthContextis a field-by-field copy ofpreview.This creates an identical object shape to
preview. If the intent is API decoupling (so auth providers don't depend on internal preview shape), this is fine. Otherwise, consider simply returning{ ...preview }to reduce surface area for drift.♻️ Optional simplification
function buildAuthContext(preview) { - return { - id: preview.id, - cmd: preview.cmd, - args: preview.args, - auth: preview.auth, - raw: preview.raw, - }; + const { id, cmd, args, auth, raw } = preview; + return { id, cmd, args, auth, raw }; }
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Tests